Skip to content

Solution#181

Open
RomanHasiuk wants to merge 3 commits intomate-academy:masterfrom
RomanHasiuk:develop
Open

Solution#181
RomanHasiuk wants to merge 3 commits intomate-academy:masterfrom
RomanHasiuk:develop

Conversation

@RomanHasiuk
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one core requirement of the task is not yet met and this affects the main functionality shown to the user. Your server currently responds to POST /add-expense with Content-Type: application/json and raw JSON; the specification requires returning an HTML page (text/html; charset=utf-8) that displays the expense as pretty-printed JSON inside something like a <pre> block, e.g. using JSON.stringify(expense, null, 2) (this is a critical functional mismatch).

Critical issues to fix:

  • In src/createServer.js, update the response for successful POST /add-expense so it sends HTML that embeds the formatted JSON instead of raw JSON with an application/json content type.
  • Make the JSON detection more robust by changing the content-type check (near where you call JSON.parse) to something like contentType && contentType.startsWith('application/json') so that values like application/json; charset=utf-8 are correctly treated as JSON.
  • Ensure public/index.html exists and contains a form that POSTs to /add-expense with fields named date, title, and amount, so the front-end requirement is clearly satisfied.

Minor but helpful improvement:

  • When writing to db/expense.json, consider using JSON.stringify(expense, null, 2) so the stored file is human-readable.

You’ve already done solid work wiring up the Node.js HTTP server, routing, validation, and file writing; once you adjust the response to be HTML, improve the content-type check, and verify the form, your solution will align well with all the requirements.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

const querystring = require('querystring');

const dataPath = path.resolve(__dirname, '..', 'db', 'expense.json');
const htmlPath = path.resolve(__dirname, '..', 'public', 'index.html');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server expects to serve public/index.html so the app can show the HTML form. Ensure public/index.html exists and contains an HTML form with date, title, and amount fields that POSTs to /add-expense (this is required by the task).

try {
const contentType = req.headers['content-type'];
const expense =
contentType === 'application/json'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing contentType === 'application/json' is brittle because headers often include a charset (for example application/json; charset=utf-8). Use a safer check like contentType && contentType.startsWith('application/json') before JSON.parse to avoid mis-detection.

Comment on lines 50 to 51
res.writeHead(200, { 'Content-Type': 'application/json' });
res.end(JSON.stringify(expense));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You currently respond with Content-Type: application/json and raw JSON. The requirement is to return an HTML page containing well-formatted JSON. Change the response to text/html; charset=utf-8 and include the pretty-printed JSON (for example JSON.stringify(expense, null, 2) inside a <pre> block) so the browser shows formatted JSON inside an HTML page.

return res.end('Missing fields');
}

fs.writeFileSync(dataPath, JSON.stringify(expense));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You write compact JSON with JSON.stringify(expense). Consider writing pretty JSON to disk using JSON.stringify(expense, null, 2) so db/expense.json is human-readable (not strictly required but helpful).

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because two core requirements of the task are not yet fully met, which will prevent the app from behaving as specified.

Critical issues that must be fixed (blockers):

  1. In src/createServer.js, when a POST /add-expense request is detected as JSON (isJson === true), you currently respond with Content-Type: application/json and raw JSON. The specification requires that all successful submissions return an HTML page that shows the pretty-printed JSON, so this branch needs to send Content-Type: 'text/html; charset=utf-8' and embed prettyJson inside a <pre> (similar to your form-encoded branch).
  2. The server’s GET handler serves public/index.html, but that file is missing from the project snapshot. You need to add public/index.html containing a form like <form method="POST" action="/add-expense"> with inputs named date, title, and amount, so that users can submit expenses through the browser.

What you did well and minor suggestions:

  • Writing db/expense.json using JSON.stringify(expense, null, 2) is correct and satisfies the “human-readable JSON file” requirement; keep that as is.
  • As a future improvement (not blocking), consider refining validation so that 0 is allowed as an amount and using async fs.writeFile instead of writeFileSync to avoid blocking the event loop.

Once you update the JSON response to return HTML and add the public/index.html form with the required fields, your solution will align well with the task requirements.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines 52 to 54
if (isJson) {
res.writeHead(200, { 'Content-Type': 'application/json' });
res.end(JSON.stringify(expense));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the request is detected as JSON (isJson true) the handler currently responds with Content-Type: application/json and raw JSON. The specification requires returning an HTML page with well formatted JSON. Change this branch so it returns an HTML page (set Content-Type: 'text/html; charset=utf-8') and embed prettyJson inside a <pre> so the JSON is displayed pretty-printed to the user.

Comment on lines +13 to +22
if (req.method === 'GET' && req.url === '/') {
fs.readFile(htmlPath, (err, data) => {
if (err) {
res.writeHead(500);

return res.end('Error loading index.html');
}
res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' });
res.end(data);
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GET handler serves public/index.html. I couldn't find public/index.html in the project snapshot; add this file. It must contain a form that POSTs to /add-expense and include inputs named date, title, and amount (for example: <form method="POST" action="/add-expense"> with name="date", name="title", name="amount"). Without it the GET / route and the form requirement will fail.

Comment on lines +48 to +50
const prettyJson = JSON.stringify(expense, null, 2);

fs.writeFileSync(dataPath, prettyJson);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good: you already create prettyJson with JSON.stringify(expense, null, 2) and write it to db/expense.json, which makes the stored file human-readable as required. Keep this behavior.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can keep progressing while you refine a few remaining details. You’ve done a solid job implementing the Node.js server: it correctly serves an HTML page, handles POST /add-expense for both form and JSON bodies, and writes pretty-printed JSON to db/expense.json.

There are a couple of important improvements to make next:

  • For JSON POST requests in src/createServer.js, when isJsonRequest is true you should respond with Content-Type: text/html; charset=utf-8 and an HTML page that embeds prettyJson inside a <pre> element, matching the form-encoded branch instead of returning raw JSON.
  • Ensure public/index.html actually exists and contains a <form method="POST" action="/add-expense"> with inputs named date, title, and amount, so browser submissions work correctly.
  • Update your validation so that 0 is accepted as a valid amount (e.g., check expense.amount == null || expense.amount === '' instead of a generic falsy check), and consider using fs.writeFile instead of fs.writeFileSync to avoid blocking the event loop.

Overall, your structure and main flow are correct; refining these details will bring your solution fully in line with the requirements and help you build good habits for production-ready Node.js code.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment on lines +54 to +56
if (isJsonRequest) {
res.writeHead(200, { 'Content-Type': 'application/json' });
res.end(JSON.stringify(expense));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: when isJsonRequest is true you send Content-Type: application/json and raw JSON. The task requires returning an HTML page that shows the pretty-printed JSON inside a <pre> element (same as the form-encoded branch). Change this branch to send Content-Type: 'text/html; charset=utf-8' and return an HTML document embedding prettyJson.

Comment on lines +14 to +21
fs.readFile(htmlPath, (err, data) => {
if (err) {
res.writeHead(500);

return res.end('Error loading index.html');
}
res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' });
res.end(data);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure the GET handler's file exists: you read and serve public/index.html here — make sure that file is present in the project and contains a <form method="POST" action="/add-expense"> with inputs named date, title, and amount, otherwise browser submissions won't work.

? JSON.parse(body)
: querystring.parse(body);

if (!expense.date || !expense.title || !expense.amount) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation issue: this check treats falsy expense.amount as missing, which will reject 0 or "0. If 0is a valid amount, change the check to test for null/empty (for example:expense.amount == null || expense.amount === ''`) instead of a simple falsy check.


const prettyJson = JSON.stringify(expense, null, 2);

fs.writeFileSync(dataPath, prettyJson);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: you use fs.writeFileSync to save the JSON which blocks the event loop. While functional, prefer fs.writeFile (async) to avoid blocking, e.g. fs.writeFile(dataPath, prettyJson, err => { ... }).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants